osd: small clear up and optimize on _recover_now and should_share_map function #13476
Conversation
|
@branch-predictor Hi! Have a look? |
src/osd/OSD.cc
Outdated
| @@ -8434,8 +8436,8 @@ void OSD::_remove_pg(PG *pg) | |||
| // and handle_notify_timeout | |||
| pg->on_removal(&rmt); | |||
|
|
|||
| service.cancel_pending_splits_for_parent(pg->info.pgid); | |||
|
|
|||
| int tr = service.cancel_pending_splits_for_parent(pg->info.pgid); | |||
There was a problem hiding this comment.
cancel_pending_splits_for_parent() does not return a value.
There was a problem hiding this comment.
@tchaikov Thank you! Sorry i mistake it.
src/osd/OSD.cc
Outdated
| @@ -8476,6 +8477,16 @@ void OSDService::_maybe_queue_recovery() { | |||
|
|
|||
| bool OSDService::_recover_now(uint64_t *available_pushes) | |||
| { | |||
| if (ceph_clock_now() < defer_recovery_until) { | |||
| dout(15) << "_recover_now defer until " << defer_recovery_until << dendl; | |||
There was a problem hiding this comment.
nit, could use __func__ here.
There was a problem hiding this comment.
@tchaikov Thank you tchaikov!I will change it later.
src/osd/OSD.cc
Outdated
| } | ||
|
|
||
| if (recovery_paused) { | ||
| dout(15) << "_recover_now paused" << dendl; |
|
and the title of your commit could be more specific. "small change and optimization" is way too vague, imo. |
src/osd/OSD.cc
Outdated
| @@ -962,6 +962,7 @@ bool OSDService::should_share_map(entity_name_t name, Connection *con, | |||
| << " versus osdmap epoch " << osdmap->get_epoch() << dendl; | |||
| if (*sent_epoch_p < osdmap->get_epoch()) { | |||
| should_send = true; | |||
| return should_send; | |||
There was a problem hiding this comment.
let's just drop should_send entirely and return true directly?
songbaisen
force-pushed
the
s18
branch
3 times, most recently
from
cdf515c
to
18906e6
Compare
songbaisen
changed the title
|
jenkins test this please |
songbaisen
force-pushed
the
s18
branch
2 times, most recently
from
4250314
to
c295dd9
Compare
| @@ -8489,15 +8499,6 @@ bool OSDService::_recover_now(uint64_t *available_pushes) | |||
| if (available_pushes) | |||
| *available_pushes = max - recovery_ops_active - recovery_ops_reserved; | |||
There was a problem hiding this comment.
Returning early will cause this function to return correct result, but leave *available_pushes unchanged, this may break code that call _recover_now and rely on available_pushes set regardless of result returned.
There was a problem hiding this comment.
@branch-predictor Agree with it good eyes man. and i change it.
| @@ -8489,15 +8499,6 @@ bool OSDService::_recover_now(uint64_t *available_pushes) | |||
| if (available_pushes) | |||
| *available_pushes = max - recovery_ops_active - recovery_ops_reserved; | |||
There was a problem hiding this comment.
Returning early will cause this function to return correct result, but leave *available_pushes unchanged, this may break code that call _recover_now and rely on available_pushes set regardless of result returned.
songbaisen
force-pushed
the
s18
branch
4 times, most recently
from
e2fa678
to
8e309aa
Compare
… function Signed-off-by: song baisen <song.baisen@zte.com.cn>
osd: small clear up and optimize on _recover_now and should_share_map function
Signed-off-by: song baisen song.baisen@zte.com.cn